Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(slo): Handle partial indicator url state #167247

Merged
merged 6 commits into from
Sep 29, 2023

Conversation

kdelemme
Copy link
Contributor

@kdelemme kdelemme commented Sep 26, 2023

resolves #166765

Summary

When the url state was partially provided, the form was set in an invalid state. This PR fixes it by merging default values with the partial provided state for every indicator type.
I've decided to only handle indicator url state, as I think this is the main use case of the feature. But we can easily enhance it with other part of the form state, e.g. objective, budgeting method, etc...

On top of that, I'm fetching the APM index whenever we render the APM indicator forms, in order to make sure we get the correct index set on the form state.

@kdelemme kdelemme self-assigned this Sep 26, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@kdelemme kdelemme added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Feature:SLO bug Fixes for quality problems that affect the customer experience labels Sep 26, 2023
@kdelemme kdelemme marked this pull request as ready for review September 26, 2023 12:43
@kdelemme kdelemme requested a review from a team as a code owner September 26, 2023 12:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/actionable-observability (Team: Actionable Observability)

@kdelemme kdelemme added the release_note:skip Skip the PR/issue when compiling release notes label Sep 26, 2023
@mgiota mgiota self-requested a review September 28, 2023 10:23
@mgiota
Copy link
Contributor

mgiota commented Sep 28, 2023

@kdelemme I noticed that the Query filter and Partition by are disabled when user lands to the SLO create form from the apm callout.

Screen.Recording.2023-09-28.at.14.18.41.mov

Is this expected behaviour?


const indicatorType = indicator.type;
switch (indicatorType) {
case 'sli.apm.transactionDuration':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It would be nice to have constants for the different indicator types. But I know we use these strings in many places, so it would require more changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are typed. If you try to use another string, or add a new type in the indicator type definition but forget to handle it here, you'll get a TS error

@kdelemme
Copy link
Contributor Author

@kdelemme I noticed that the Query filter and Partition by are disabled when user lands to the SLO create form from the apm callout.

Screen.Recording.2023-09-28.at.14.18.41.mov
Is this expected behaviour?

Indeed, It is expected because the Transaction Name is not selected (or provided by the url state from the APM page).

@kdelemme kdelemme requested a review from mgiota September 28, 2023 13:20
@mgiota
Copy link
Contributor

mgiota commented Sep 28, 2023

@kdelemme Same happens when the Transaction Name is selected. I'll check how the form validation is done.
Screenshot 2023-09-28 at 22 04 13

@kdelemme
Copy link
Contributor Author

I don't have this behaviour... let me investigate further.

@mgiota
Copy link
Contributor

mgiota commented Sep 28, 2023

@kdelemme In my case index is undefined and that's why query filter and partition by fields are disabled. I'll try again, maybe I do something wrong. But I guess we need to set the index param when we click on the apm call out.

When I create the SLO from the SLO page, index is metrics-apm*, apm-*

Screenshot 2023-09-28 at 22 35 38

@kdelemme
Copy link
Contributor Author

Maybe a race condition, I need to take a closer look at this. But does the indicator form works when not coming from a prefilled url?

@mgiota
Copy link
Contributor

mgiota commented Sep 28, 2023

Yes it works, when not coming from a prefilled url. I can file a separate ticket for this issue with clear steps to reproduce and we fix there. I can have a look.

Here's the ticket #167630. If I hardcode the index here, it works: the query filter and the partition by fields are not disabled. So I just need to figure out how to dynamically get the index name. I am not sure if I want to send an index param from the apm page or set directly the index in the apm_availability_indicator_type_form.tsx if param index is undefined. Most probably the 2nd option. What do you think?

@kdelemme
Copy link
Contributor Author

@mgiota Actually if the index is supposed to be automatically filled by the form, if using the url state to prefill the form breaks this, we should not merge this PR. I'm going to investigate further and implement the fix in this PR

@kdelemme kdelemme marked this pull request as draft September 29, 2023 12:19
@kdelemme kdelemme marked this pull request as ready for review September 29, 2023 13:13
@kdelemme kdelemme requested a review from mgiota September 29, 2023 13:13
@kdelemme
Copy link
Contributor Author

@mgiota Good find with the bug! I've fixed it fetching the apm index when the form renders.
I've tested the following scenarios:

  1. From the APM page using the URL state
  2. From the Create SLO button, selecting APM Availability then APM latency
  3. From the Edit SLO button

If you want to see what is contained in the form at any time, I use this in the main slo_edit_form component:

<pre>{JSON.stringify(watch('indicator'), null, 2)}</pre>

Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm! You can update the description of this ticket and include resolves #167630

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 1.0MB 1.0MB +784.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kdelemme

@kdelemme kdelemme merged commit 9d3213e into elastic:main Sep 29, 2023
@kdelemme kdelemme deleted the fix/slo-partial-url-state branch September 29, 2023 14:19
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.10 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.10:
- feat(slo): add partition by field in APM indicators (#165106)

Manual backport

To create the backport manually run:

node scripts/backport --pr 167247

Questions ?

Please refer to the Backport tool documentation

@kdelemme
Copy link
Contributor Author

As soon as this backport is merged, I'll be able to backport this PR into 8.10 as well.

@kdelemme
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.10

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

1 similar comment
@kdelemme
Copy link
Contributor Author

kdelemme commented Oct 1, 2023

💚 All backports created successfully

Status Branch Result
8.10

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kdelemme added a commit to kdelemme/kibana that referenced this pull request Oct 1, 2023
kdelemme added a commit to kdelemme/kibana that referenced this pull request Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience Feature:SLO release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't create an SLO through the banner link on the APM Service overview
6 participants